-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Webauthn HID client #12
base: master
Are you sure you want to change the base?
Conversation
- add a CLI webauthn implementation, trying to match as close as possible browsers behavior. - includes device selection when multiple authenticators are plugged in. - includes working examples on webauthn.io and demo.yubico.com public webauthn servers, see webauth/example package. - support usb authenticators, using either FIDO U2F/CTAP1 or CTAP2 protocols
pin input now use golang.org/x/crypto/ssh/terminal to not be echoed on stdout anymore. The library is expected to work on *any* go supported OS terminals, but only tested on linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting my incomplete review from a few months ago. Will do another pass later.
BaseIV []byte `cbor:"5,keyasint,omitempty"` | ||
} | ||
|
||
func (k *COSEKey) CBOREncode() ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (k *COSEKey) CBOREncode() ([]byte, error) { | |
func (k *COSEKey) MarshalCBOR() ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't rename, this cause infinite loop as MarshalCBOR() makes the struct implement cbor.Marshaler interface (see https://github.com/fxamacker/cbor/blob/ee962ff86de23bc964f19d8e4a1a23171b05e58b/encode.go#L28-L29 and https://github.com/fxamacker/cbor/blob/ee962ff86de23bc964f19d8e4a1a23171b05e58b/encode.go#L99-L101)
return getpasswd(h.Stdin) | ||
} | ||
|
||
func (h *InteractiveHandler) SetPIN(token *ctap2token.Token) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like most of this handler should be abstracted out into a helper that other handlers can use? I'd expect it to only do the collection from stdin and then call a shared function that does all the work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done.
ctap2token/token.go
Outdated
} | ||
|
||
// MakeCredentialResponse | ||
// TODO: structure may be different with different kind of attestations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this TODO still pending?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm no. The MakeCredentialResponse structure is fixed and only the content of AttSmt may vary, but since it's a map[string]interface{} we should be good to store anything. Thanks, removed.
Co-authored-by: Jonathan Rudenberg <[email protected]>
HyperSecu Mini tokens are reporting errors when calling GetAssertion. It seems these tokens don't support the transports field being set on the CredentialDescriptor. Luckily it's optionnal so we can safely remove it.
HyperSecu mini tokens returns wrong error on timeout that we can catch in order to avoid asking for user pin when it's not set
No description provided.